-
Notifications
You must be signed in to change notification settings - Fork 389
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(new): check for installed package managers #2495
feat(new): check for installed package managers #2495
Conversation
Hey @kamilmysliwiec, can you check this out? |
actions/new.action.ts
Outdated
stdio: 'ignore', | ||
}); | ||
return true; | ||
} catch (error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
} catch (error) { | |
} catch { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actions/new.action.ts
Outdated
if (isThisPackageManagerInstalled('npm')) | ||
installedPackageManagers.push(PackageManager.NPM); | ||
if (isThisPackageManagerInstalled('yarn')) | ||
installedPackageManagers.push(PackageManager.YARN); | ||
if (isThisPackageManagerInstalled('pnpm')) | ||
installedPackageManagers.push(PackageManager.PNPM); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (isThisPackageManagerInstalled('npm')) | |
installedPackageManagers.push(PackageManager.NPM); | |
if (isThisPackageManagerInstalled('yarn')) | |
installedPackageManagers.push(PackageManager.YARN); | |
if (isThisPackageManagerInstalled('pnpm')) | |
installedPackageManagers.push(PackageManager.PNPM); | |
if (isThisPackageManagerInstalled('npm')) { | |
installedPackageManagers.push(PackageManager.NPM); | |
} | |
if (isThisPackageManagerInstalled('yarn')) { | |
installedPackageManagers.push(PackageManager.YARN); | |
} | |
if (isThisPackageManagerInstalled('pnpm')) { | |
installedPackageManagers.push(PackageManager.PNPM); | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure how I feel about this change. Isn't this confusing that some options just won't show up for certain people? Like one might be aware that yarn is a thing but doesn't have it installed on let's say their machine at work. Then they generate a NestJS app and they only see NPM showing up which will make them think using yarn was never an option (simply because it's hidden). Shouldn't we just leave this decision to the developer since the developer should be aware of what tool they have installed (and even if they don't they can always terminate the command, install it, and start over) and want to use for their projects? |
Yes, this is a bit of a "divisive" change, to one 50 percent it can be helpful while the other 50 percent can be confusing 😄 |
I made this change after I realize there is an error when you continue with yarn even yarn is not installed, as below. If it's not a good idea to hide package managers, it might be a better idea to warn the user if, for example, yarn is not installed, but to build the project (without installing packages). |
Even if the installation fails, everything else should be properly generated and you should be good to go as long as you manually install dependencies |
Yes, I mean the error is not clear. In my firsr use of nest-cli, I thought project wasn't built. |
So maybe instead of hiding package managers we should rather improve our error messages; let users know that even if the installation failed they're generally (almost) all set |
Agree. Should I close this PR and open a new one, or continue on this? |
Feel free to continue in this PR |
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Currently, the CLI tool asks for all package managers (npm, yarn, and pnpm) even if they aren't installed.
What is the new behavior?
Now, CLI checks for installed package managers and then asks for those that are installed, not for the ones that are not installed.
Does this PR introduce a breaking change?